-
Notifications
You must be signed in to change notification settings - Fork 21
feat:manage user task #242
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
| Category | Issue | Status |
|---|---|---|
| Incomplete Task Action Handling ▹ view | ✅ Fix detected | |
| Sequential Async Storage Reads ▹ view | ✅ Fix detected | |
| Incomplete Task Completion Implementation ▹ view | ✅ Fix detected | |
| Inefficient Date Operations in Loop ▹ view | ✅ Fix detected | |
| Poor Story Implementation Pattern ▹ view | ✅ Fix detected | |
| Missing Mock Data Implementation ▹ view | ✅ Fix detected | |
| Embedded Sample Data ▹ view | ✅ Fix detected |
Files scanned
| File Path | Reviewed |
|---|---|
| packages/good-design/src/apps/usertasks/ManagerTaskCards.stories.tsx | ✅ |
| packages/good-design/src/apps/usertasks/managerTaskCard.tsx | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Check out our docs on how you can make Korbit work best for you and your team.
| actionUrl?: string; | ||
| } | ||
|
|
||
| const SAMPLE_TASKS: ClaimerTask[] = [ |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| } | ||
|
|
||
| export const ClaimerTasksCard: React.FC<ClaimerTasksCardProps> = ({ onTaskDismiss, fontStyles }) => { | ||
| const { mainTask, secondaryTasks, loading, hasActiveTasks, dismissTask } = useClaimerTasks(); |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| const openTask = async (task: ClaimerTask) => { | ||
| if (task.actionUrl) { | ||
| try { | ||
| await Linking.openURL(task.actionUrl); | ||
| } catch (error) { | ||
| console.warn("Failed to open task URL:", error); | ||
| } | ||
| } | ||
| }; |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| const dismissed = await AsyncStorage.getItem("dismissed_claimer_tasks"); | ||
| const completed = await AsyncStorage.getItem("completed_claimer_tasks"); |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| const availableTasks = useMemo(() => { | ||
| const now = new Date(); | ||
| const fourDays = 4 * 24 * 60 * 60 * 1000; | ||
|
|
||
| return SAMPLE_TASKS.filter(task => { | ||
| const start = new Date(task.duration.startDate); | ||
| const end = new Date(task.duration.endDate); | ||
| if (now < start || now > end) return false; | ||
| if (completedTasks.includes(task.id)) return false; | ||
|
|
||
| const dismissed = dismissedTasks[task.id]; | ||
| if (dismissed && now.getTime() - dismissed < fourDays) return false; | ||
|
|
||
| return true; | ||
| }); | ||
| }, [dismissedTasks, completedTasks]); |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| export const WithMockData = () => ( | ||
| <NativeBaseProvider> | ||
| <ClaimerTasksCompact | ||
| //tasks={tasksData} |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| export const WithMockData = () => ( | ||
| <NativeBaseProvider> | ||
| <ClaimerTasksCompact | ||
| //tasks={tasksData} | ||
| onTaskComplete={taskId => console.log("Task completed:", taskId)} | ||
| onTaskDismiss={taskId => console.log("Task dismissed:", taskId)} | ||
| /> | ||
| </NativeBaseProvider> | ||
| ); |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Hi @vortex-hue! I opened this draft since our tickets are related, thought it might help us both. If you’d like to work on the same branch or need any help, just let me know! |
Hi Victor, Thanks for the draft, am open to working together on this, are you available for a meet tomorrow? |
|
@vortex-hue Sure. How can I reach out to you? |
+2348115333313 WhatsApp or @devops_pete on telegram |
|
You've used up your 5 PR reviews for this month under the Korbit Starter Plan. You'll get 5 more reviews on August 19th, 2025 or you can upgrade to Pro for unlimited PR reviews and enhanced features in your Korbit Console. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @YanVictorSN - I've reviewed your changes - here's some feedback:
- The
tasksprop on ClaimerTasksCard is never used—refactor the hook/component to accept injected task lists instead of always relying on SAMPLE_TASKS. - Consider extracting the date‐range and dismissal filtering logic into a pure utility function for easier testing and reuse outside the hook.
- Repeated AsyncStorage writes on each dismiss or complete action could impact performance—consider debouncing or batching persistence updates.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `tasks` prop on ClaimerTasksCard is never used—refactor the hook/component to accept injected task lists instead of always relying on SAMPLE_TASKS.
- Consider extracting the date‐range and dismissal filtering logic into a pure utility function for easier testing and reuse outside the hook.
- Repeated AsyncStorage writes on each dismiss or complete action could impact performance—consider debouncing or batching persistence updates.
## Individual Comments
### Comment 1
<location> `packages/good-design/src/apps/usertasks/managerTaskCard.tsx:26` </location>
<code_context>
+ actionUrl?: string;
+}
+
+export const useClaimerTasks = () => {
+ const [dismissedTasks, setDismissedTasks] = useState<{ [key: string]: number }>({});
+ const [completedTasks, setCompletedTasks] = useState<string[]>([]);
</code_context>
<issue_to_address>
The useClaimerTasks hook does not use the 'tasks' prop from ClaimerTasksCard.
Since useClaimerTasks always uses SAMPLE_TASKS, any 'tasks' prop passed to ClaimerTasksCard is ignored. Refactor useClaimerTasks to accept a tasks argument or clarify the intended usage to avoid confusion or bugs.
</issue_to_address>
### Comment 2
<location> `packages/good-design/src/apps/usertasks/managerTaskCard.tsx:76` </location>
<code_context>
+ }
+ };
+
+ const dismissAllTasks = async (taskIds: string[], onTaskDismiss?: (taskId: string) => void) => {
+ setDismissing(true);
+ const now = Date.now();
</code_context>
<issue_to_address>
Potential race condition when dismissing all tasks due to state update timing.
Since setDismissedTasks is asynchronous, the state may not be updated before persisting to AsyncStorage. Use a functional update or ensure the latest state is saved.
</issue_to_address>
### Comment 3
<location> `packages/good-design/src/apps/usertasks/mockData.ts:10` </location>
<code_context>
+ category: "engagement",
+ priority: "main",
+ reward: { type: "tokens", amount: 10, description: "Vote on" },
+ duration: { startDate: "2025-07-23", endDate: "2025-08-23" },
+ actionUrl: "https://www.gooddollar.org/"
+ },
</code_context>
<issue_to_address>
Sample task dates are set in the future, which may affect development and testing.
Using fixed future dates may hinder testing of time-dependent features. Consider switching to relative dates or document this choice for clarity.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| actionUrl?: string; | ||
| } | ||
|
|
||
| export const useClaimerTasks = () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (bug_risk): The useClaimerTasks hook does not use the 'tasks' prop from ClaimerTasksCard.
Since useClaimerTasks always uses SAMPLE_TASKS, any 'tasks' prop passed to ClaimerTasksCard is ignored. Refactor useClaimerTasks to accept a tasks argument or clarify the intended usage to avoid confusion or bugs.
| } | ||
| }; | ||
|
|
||
| const dismissAllTasks = async (taskIds: string[], onTaskDismiss?: (taskId: string) => void) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (bug_risk): Potential race condition when dismissing all tasks due to state update timing.
Since setDismissedTasks is asynchronous, the state may not be updated before persisting to AsyncStorage. Use a functional update or ensure the latest state is saved.
Description
A component that show claimers a set of tasks they can complete.
About #228
Description by Korbit AI
What change is being made?
Add a new
ClaimerTasksCardcomponent to manage and display user tasks with capabilities to mark tasks as complete or dismissed and integrate this with existing stories.Why are these changes being made?
These changes address the need to manage user engagement by allowing users to interact with tasks, enhancing the user experience by providing clear task rewards and statuses. This approach utilizes
Reactandnative-basefor robust UI management, with efficient state handling using hooks. However, reliance onAsyncStoragemay warrant future revisits for optimized performance.